Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Fix theme scoping issues #10498

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

yaredtsy
Copy link
Contributor

@yaredtsy yaredtsy commented Sep 27, 2023

Fixes #10484

Reverts the change done in #10437

Problem

Following the update to #8032, certain components that utilize 'styled' from '@mui/system' alongside HTML elements experience crashes when used with theme scoping. This is due to the fact that 'styled' and HTML elements are unaware of theme scoping and attempt to access the theme at the root object.

Demo: https://codesandbox.io/s/nice-moore-wsp7q8?file=/src/demo.tsx

@mui-bot
Copy link

mui-bot commented Sep 27, 2023

Deploy preview: https://deploy-preview-10498--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against c0463fd

@michelengelen michelengelen added component: data grid This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Sep 28, 2023
@DanailH DanailH changed the title [DataGrid] fix theme scoping issues [DataGrid] Fix theme scoping issues Sep 29, 2023
Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know enough about theming to approve this one.

Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating the PR with the suggested approach. 🙏

packages/grid/x-data-grid/src/styles/styled.ts Outdated Show resolved Hide resolved
@michelengelen michelengelen changed the base branch from master to next November 6, 2023 14:22
@MBilalShafi
Copy link
Member

@cherniavskii Would appreciate it if you could take a quick look and see if the fix looks good to you 🙏

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

import { createStyled } from '@mui/system';

// hardcode theme id to avoid importing from `@mui/material`
export const styled = createStyled({ themeId: '$$material' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: should we export it publicly? Which styled should devs use when e.g. creating a custom slot for the Data Grid?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They could use the one from @mui/material/styles, no? IIANM, the need to de-isolate material/styles is more for internal use.
We could also expose this one and ask them to use it but it might be difficult to effectively communicate this to the users. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to switch from @mui/system to use styled from @mui/material/styles for now?

I recall that the use of @mui/system is to support Joy in the future but since the core team is focusing on v6, we could revisit and provide a better solution in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to work on bundling the grid without MaterialUI this quarter though. Which implies we remove any @mui/material import from the core of the grid's codebase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to switch from @mui/system to use styled from @mui/material/styles for now?

Thanks for pitching in @siriwatknp

I would agree with @romgrk and possibly avoid reverting to an import from @mui/material. IMO, this workaround could be a nice compromise for the time being.

Or do you see a problem with this other than this duplicated hardcoded import?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 16, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 16, 2024
@romgrk
Copy link
Contributor

romgrk commented Aug 16, 2024

I have picked up this PR and will replace a few missing imports that were added in the meantime.

Comment on lines +60 to 61
// @ts-ignore `@mui/material` system styled() doesn't have a compatible sx prop
<GridFooterCellRoot ownerState={ownerState} className={classes.root} {...other}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using system styled() for GridFooterCellRoot means it's incompatible because we explicitly declare the type as sx?: MaterialTheme.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fixed by my suggestion in internals/styled:

createStyled<Theme>({ themeId: MATERIAL_THEME_ID });

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 16, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 21, 2024
@romgrk
Copy link
Contributor

romgrk commented Aug 28, 2024

I realized there is a problem with this approach. The styled from material is also injecting material's default theme, which has a few more fields than the system one and is otherwise not accessible. As far as I understand, we'd need to require that context to be passed through material's ThemeProvider to get rid of that dependency. So this might be a blocker for the design-system agnostic OKR.

Maybe there's a way to get around that by providing ThemeProvider with a default material theme in a DataGridMaterial wrapper, but this PR would get more complex than I hoped it would be. Something like this:

const materialDefaultTheme = createTheme() // from material
function DataGridMaterial(props) {
  const theme = useTheme() ?? materialDefaultTheme
  return (
    <ThemeProvider theme={theme}>
      <DataGrid {...props} />
    </ThemeProvider>
  )
}

Thoughts/ideas @mui/xgrid?

Comment on lines +42 to +50
// @ts-ignore `@mui/material` theme.typography does not exist
fontSize: theme.typography.caption.fontSize,
// @ts-ignore `@mui/material` theme.typography does not exist
lineHeight: theme.typography.caption.fontSize,
position: 'absolute',
bottom: 4,
// @ts-ignore `@mui/material` theme.typography does not exist
fontWeight: theme.typography.fontWeightMedium,
// @ts-ignore `@mui/material` theme.vars does not exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure how we should evolve these styles that rely on material theme. Maybe we should define a minimal default grid theme with the styles we need from material? And then we document which theme fields need to be re-defined for other design systems?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a good approach, I would even recommend introducing CSS vars for these tokens that reads from the theme if the value exists, or fallback to some default value. That way people don’t need to define theme, they can just add CSS vars instead.

Copy link
Member

@siriwatknp siriwatknp Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for creating DataGrid CSS variables. It can be a plain object for type safety, basically to remove accessing the JS theme. Then in the, GridRootStyles declare the values using theme values from Material UI:

// dataGridCssVars.ts
// the structure below is just an example.
export const vars = {
  typography: {
    caption: {
      fontSize: '--DataGrid-typography-caption-fontSize',}
  }
}

// GridAggregationHeader
fontSize: `var(${vars.typography.caption.fontSize}, 12px)`, // the fallback is optional

// GridRootStyles.ts
const gridStyle: CSSInterpolation = {
  [vars.typography.caption.fontSize]: t.typography.caption.fontSize,
  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on adding those properties on the system theme object instead?

One issue I have with CSS variables is that it's going to be a lot of boilerplate to redefine all the theme variables we need. And once we have released publically a set of CSS variables, it gets harder to add new ones because it could be a breaking change if that new variable is not defined by users, whereas the theme object covers basically every theming/styling use-case.

If we go with CSS variables, I'm also unsure if/how to adapt patterns like these ones:

  • padding: theme.spacing(0.5)
  • [theme.breakpoints.up('sm')]: { ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And once we have released publically a set of CSS variables, it gets harder to add new ones because it could be a breaking change if that new variable is not defined by users

In the case of adding new variables, would we not provide a default in the Data Grid, and then users only need to define it if they wish to override the value? That should prevent breaking changes unless I'm misunderstanding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on adding those properties on the system theme object instead?

What does this mean in practice? We extend the system theme by adding the properties we need, and then each consumer is responsible for providing a compatible theme object, correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of adding new variables, would we not provide a default in the Data Grid, and then users only need to define it if they wish to override the value?

Yes in theory, in practice I feel like there could be cases where we can't find a good default.

What does this mean in practice? We extend the system theme by adding the properties we need, and then each consumer is responsible for providing a compatible theme object, correct?

Yes, that would be the idea. The theme object is more structured and already has typings and everything needed for theming.

If we go to CSS variables, we'd need to replace pretty much all of these cases with them (unless we only use CSS variables for the cases where the system theme object doesn't match the material theme, but mixing both paradigms sounds bad).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be the idea. The theme object is more structured and already has typings and everything needed for theming.

True, that is a valid approach and a practical one.

If we go to CSS variables, we'd need to replace pretty much all of these cases with them (unless we only use CSS variables for the cases where the system theme object doesn't match the material theme, but mixing both paradigms sounds bad).

Moving CSS will take more effort. I think it will involve a design engineer to come up with a minimal set of variables that will be used across the data grid in an efficient way (might include a redesign).

But I think it's a long term way because it does not involve JS theme anymore, it's just CSS and anyone can override theme (theming the data grid) with plain CSS.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 29, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

// Hardcoded theme id to avoid importing from `@mui/material`
const MATERIAL_THEME_ID = '$$material';

export const styled = createStyled({ themeId: MATERIAL_THEME_ID });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const styled = createStyled({ themeId: MATERIAL_THEME_ID });
export const styled = createStyled<Theme>({ themeId: MATERIAL_THEME_ID });

The Theme should be the Material UI Theme + other properties that data grid wants to extend.

@@ -40,11 +39,15 @@ const GridAggregationFunctionLabel = styled('div', {
overridesResolver: (_, styles) => styles.aggregationColumnHeaderLabel,
})<{ ownerState: OwnerState }>(({ theme }) => {
return {
// @ts-ignore `@mui/material` theme.typography does not exist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the @ts-ignore will be removed by my suggestion https://github.com/mui/mui-x/pull/10498/files#r1778208301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] should work with theme scoping (e.g. theme-ui)
9 participants